This looks like we should pay more attention to this when reviewing changes to the unified test format as it was previously missed.

I noticed this file is quite outdated. Since DRIVERS-1767 was implemented and a CI workflow was added to test spec files against their exact schemaVersion, this file only serves two purposes:

• Validate a wide number of specs against the latest schema (not sure how useful or necessary this is)
• Test schema validation failures for invalid tests, which are ignored by the CI workflow

FWIW, the Makefile is still quite a bit faster than the check_schema_version.sh script since it invokes ajv once for an entire directory of files. Going forward, I think it'd be best to eliminate this file and update check_schema_version.sh to also process invalid tests and possible optimize it to invoke fewer instances of ajv (e.g. collect specs per schema and run a single ajv instance per schema).

Tracking in DRIVERS-2466.

This test covers the original impetus for this functionality (see: DRIVERS-2353). The other tests for aggregate and findOneAndUpdate are meant to provide some additional test coverage without being exhaustive.

oh boy I get to do another rebase and schema version bump on #1303 🙂

In all seriousness, if your PR is nearly ready to merge I'm happy to hold off on this for a few more days and spare you the trouble.

ha, I appreciate it, but it really isn't too big of a deal. I am really hoping my PR is going to get merged this week but I don't want to promise it. so let's just let things play out naturally timing wise

it seems we're treating this as a root document. should we call that out here or in Evaluating Matches?

I added errorResponse to the non-exhaustive list in "Allowing Extra Fields in Root-level Documents" and also made a note that it should be matched as a "root-level document" here so there's no ambiguity.

FWIW, it doesn't look like we did that for any other instances. Any spec test value directly matched using the "Evaluating Matches" rule is assumed to be a root-level document. The rules for non-root-level matching only kick in when encountering an embedded document within one of those values.

oh bulk is a very interesting case. I guess if the server starts adding arbitrary extra fields to write command responses we might have to do some work to figure out how to expose those via bulk results.

also, somewhat of an aside but interestingly, the scope for the new server bulk write command proposes that results are returned via a cursor (this is to handle situations where there are too many errors to fit in a single response, which can happen now sometimes for unordered bulk writes and causes an exception in the server).
if a driver did choose to add a rawResponses property or something to their BulkWriteFailure they would have to consider how to handle that upcoming case as well.

After POCing this, there's certainly some ambiguity in which error is the errorResponse error in some situations.

Bulk write is one of them. Why are we advising against testing errorResponse with bulk writes here and then adding a test for it here? Am I misunderstanding something? I went with a last-write-error-becomes-the-errorResponse approach (which works since there's only one error), but that was arbitrary.

There's further ambiguity in that the Go driver's normal WriteException can have multiple WriteErrors with multiple raw server responses when we're batching writes. Furthermore, WriteException and BulkWriteException both contain WriteConcernErrors each with their own raw response in the Go driver.

I guess if the server starts adding arbitrary extra fields to write command responses we might have to do some work to figure out how to expose those via bulk results.

That thought just occurred to me this morning while I was revisiting the PHPLIB POC. I noticed that DuplicateKey(11000) returns extra fields in the write error document, which may be inaccessible in languages that model WriteError and WriteConcernError. See: DRIVERS-2470.

Why are we advising against testing errorResponse with bulk writes here and then adding a test for it here? Am I misunderstanding something? I went with a last-write-error-becomes-the-errorResponse approach (which works since there's only one error), but that was arbitrary.

My intention was to provide as much test coverage as possible for drivers that can support this, and encourage incompatible drivers to skip them. I intentionally had a single operation in the bulkWrite-errorResponse test to avoid ambiguity, assuming that any exception would inherit the server-side error code from the insert command. If that seems problematic I'm happy to delete the test. Alternatively, I can add a comment explaining that things could be ambiguous with multiple operations in the bulk and note my intention for only performing a single insert.

Regarding WriteError and WriteConcernError, I don't think those come into play here since errorResponse is only asserting the top-level command response. Technically, we should be able to assert the presence of gossiped cluster time if so inclined.

Alternatively, I can add a comment explaining that things could be ambiguous with multiple operations in the bulk and note my intention for only performing a single insert.

That sounds good to me. I can see how these tests are really just to encourage drivers to expose the raw server response wherever they can.

Regarding WriteError [assume you mean extra write errors on non-bulk operations] and WriteConcernError, I don't think those come into play here since errorResponse is only asserting the top-level command response.

Ah gotcha. Sounds good 👍

Added a comment to the bulk and non-bulk tests in 1415116, which explains that some drivers may need to skip them. The CRUD spec doesn't go into much detail about how to construct BulkWriteException or WriteException beyond collecting write and write concern errors.

assume you mean extra write errors on non-bulk operations

I wasn't distinguishing between bulk and non-bulk writes, since the write commands all return writeErrors in an array. Rather, the fail point I'm using in these tests sets the top-level error code for the command (and forces an ok: 0 response). That is all errorResponse is asserting here.

If we wanted, we could theoretically use errorResponse to peak inside of the writeErrors array (for example), but I didn't see the need for that.

After POCing this, there's certainly some ambiguity in which error is the errorResponse error in some situations.

Bulk write is one of them. Why are we advising against testing errorResponse with bulk writes here and then adding a test for it here? Am I misunderstanding something? I went with a last-write-error-becomes-the-errorResponse approach (which works since there's only one error), but that was arbitrary.

There's further ambiguity in that the Go driver's normal WriteException can have multiple WriteErrors with multiple raw server responses when we're batching writes. Furthermore, WriteException and BulkWriteException both contain WriteConcernErrors each with their own raw response in the Go driver.

The Go driver does not have a modifyCollection helper, so I can't run this test 😢 .

For the benefit of test coverage, have you considered implementing the operation via runCommand in your test suite? PHP has a helper for it but we don't actually model all of the possible parameters, so it should be quite trivial to just build a command document from the arguments in the test operation if you wanted.

Maybe not so relevant for this case, since the purpose of the test is to ensure a helper provides appropriate error access, but it could allow you to run some other tests that happen to use modifyCollection.

Yeah an interesting point; I imagine users are already running collMod through our RunCommand API, so would be good to test it. Filed GODRIVER-2587.